Report PII-free error templates in deploy telemetry - #6355

…tests

The inline `jq 'select(...)' out.requests.txt` pipelines duplicated what
print_migration_telemetry and print_telemetry_string_values already print
from the same recorded request, and .agents/rules/testing.md calls that
pipeline out as an anti-pattern precisely because the helpers exist.

Removing out.telemetry.txt also drops a synced file, hence the Files:
count in the migrate golden.

Co-authored-by: Isaac

…g map

The string_values map was justified as mirroring bool_values so later keys
would need no proto change, but adding the map is itself a proto change, so
that cost is paid either way and the flexibility only covers keys that do
not exist. Three known slots are better as named fields: a typo is a
compile error, the struct documents what exists, and the backend gets real
columns instead of a map to explode.

Splitting the migration template in two also drops a join: the field names
match the booleans recorded beside them, so a conversion failure is
distinguishable from a commit failure without consulting bool_values.

Co-authored-by: Isaac

Every resource key starts with it, so it carries no information in a
template: "jobs.*" says what "resources.jobs.*" said.

Co-authored-by: Isaac

The same two values were rendered in opposite orders by two functions in
the same package: the user-facing summary reads "(403 INJECTED)" and the
template read "INJECTED 403".

Co-authored-by: Isaac

A filer error is a fixed classification plus the path it concerns:
"access denied: /Workspace/...". The classification is a source literal and
safe to report; the path is not. Naming the literals lets Error() and
SafeString() derive from one string so they cannot drift, and leaves all 47
construction sites untouched.

safeerr consults a wrapped error's stand-in under %w when it has no template
of its own, and diag.ErrorTemplate consults it at the top of the chain, so
this reports usefully from the call sites not yet raised through safeerr —
which is most of them:

  before  pushing direct state to workspace: %w [403 INJECTED]
  after   pushing direct state to workspace: access denied [403 INJECTED]

It matters most where the HTTP status cannot discriminate: a missing file and
a missing parent directory are both 404.

Co-authored-by: Isaac

Only the auto-migrate tests asserted what migration telemetry a deploy
reports; the rest exercised migration scenarios while the telemetry went
unobserved. Every migration test that records requests now prints it, so a
change in what a scenario reports shows up as a golden diff.

print_migration_telemetry collapses identical lines, since out.requests.txt
can hold several deploys and repeating a line per deploy says nothing, and
reports missing requests rather than letting jq complain about the file.

Four tests are left out: default-python, profile_arg and var_arg set
RecordRequests = false, and the two engine-config tests run only a failing
"deployment migrate", which is not a deploy and emits no event.

Also table-tests the conversion failures in build_state_test.go, which only
covered success paths, pinning each message against its template.

Co-authored-by: Isaac

…ploy

Rather than collapsing repeated lines, each block is printed right after the
deploy it belongs to and the recorded requests are cleared, so a block covers
exactly one deploy.

default-python, profile_arg and var_arg now record requests too, and remove
them once the telemetry is printed, so nothing large lands in a golden.
default-python needs withdir: its script cd's into the generated bundle while
the requests are recorded at the test root.

Three blocks come out empty on purpose. Those deploys run after the state is
already direct, and MigrateToDirect is skipped entirely for a direct deploy,
so the empty block is the assertion.

Co-authored-by: Isaac

Deploy errors are a separate concern from migration, so error_template and the
plumbing it needed are gone: diag.Diagnostic's field, logdiag's
FirstErrorTemplate, Metrics.ErrorTemplate, the proto field and the
deploy-error-template test. That also reverts the exhaustruct additions in
load_dbalert_files.go. Only direct_migrate_error_template and
direct_migrate_commit_error_template remain.

Telemetry logging is reverted from the migration tests where it only showed
direct_drymigrate_success true, which fifteen other tests already assert.

Nothing covered the conversion failure class, only commit failures. The new
auto-migrate-conversion-failure test uses an unknown secret scope permission
level: a warning for the terraform deploy, but SecretScopeFixups rejects it
while preparing the config for the direct engine. It asserts both populations
— a failed dry run when not opted in, and a stopped migration when opted in.
A conversion failure makes no API call, so unlike the commit failures it
cannot be produced by injecting a fault.

Co-authored-by: Isaac

The fixups ran through their mutator, so the failure arrived as a diagnostic
and the migration could only report a generic "failed to apply secret scope
fixups" — losing which fixup failed, and printing two warnings for one
problem.

ApplySecretScopeFixups is now the body, returning a real error and the
offending scope key, with the mutator as a wrapper around it. The migration
calls it directly, so its safeerr template survives:

  before  direct_migrate_error_template  failed to apply secret scope fixups
  after   direct_migrate_error_template  unknown permission level %q for secret scope

Only the level itself is withheld; the rest of the message is a literal.

Co-authored-by: Isaac

#6368 regenerated the same auto-migrate goldens for its output reordering.
Since main carries no template field, the rebase auto-merge took its line set
and dropped direct_migrate_commit_error_template from both.

Co-authored-by: Isaac

BuildStateFromTF wrapped node in config.ResourceKey at all twelve of its error
sites. Retyping the loop variable would only trade those for seven string(node)
casts at the places that want a plain string, so derive the key once instead.

Co-authored-by: Isaac

One is a genuine failure: a terraform.tfstate whose format version the
migration does not understand. It is only reachable through "bundle deployment
migrate", since a deploy would have terraform reject the state first, so this
one records no telemetry.

The other three record that shapes which look like they should break do not:

  reference-terraform-syntax    ${databricks_job.src.id} deploys and migrates
                                cleanly; reverseInterpolate maps it back and
                                both resolution methods agree
  reference-direct-only-resource an app env var referencing a catalog migrates
                                cleanly: apps.config is inline app.yaml rather
                                than an API field, so ExtractReferences drops
                                the reference before resolving it
  reference-dabs-only-field     a reference to a DABs-only field never reaches
                                the migration at all — Interpolate rewrites it
                                to a Terraform attribute that does not exist and
                                the deploy fails first

They are regression cover: each asserts the current outcome, so a change that
makes one start failing shows up here.

Co-authored-by: Isaac

A warning stops an automatic migration exactly as an error does, but carries no
error to describe, so direct_migrate_warnings was a bare boolean with nothing
saying which warning it was — for the outcome that is plausibly the most common
blocker, since a single disagreeing field is enough.

BuildStateFromTF now also returns a PII-free description of the first warning,
built through safeerr from the parts that are safe: the resource type and the
stage, never the resource name or the disagreeing values. It covers both warning
sources, a resolution disagreement and a resource type with no adapter, and
lands in direct_migrate_warning_template.

Unit-tested only: neither warning is reachable from an acceptance test, since a
disagreement needs a crafted terraform.tfstate and every terraform resource type
currently has an adapter.

Co-authored-by: Isaac

… tests

The three metrics are now direct_migrate_saferr, direct_migrate_commit_saferr
and direct_migrate_warning_saferr, naming the machinery that produces them
rather than the shape of the value.

tfstate-version-unsupported now exercises the automatic migration instead of
the manual command. A post-deploy script bumps the state format version, which
the deploy runs after applying and before the migration, so the deploy succeeds
against the state it wrote and only the conversion fails. It reports
direct_migrate_error, and now a saferr too: the version error was raised with
fmt.Errorf, so nothing described it.

reference-terraform-syntax now performs a real migration rather than a dry run,
which exposed a defect: BuildStateFromTF derives depends_on from each reference
via GetNodeAndType, which returns an empty node for a path that does not start
with "resources". The empty node lands in resources.json, the migration reports
success, and the next plan fails with `invalid dependency ... no such node ""`.
The test records that rather than fixing it, since the fix is a behaviour change
that does not belong in a telemetry PR.

Also: reference-dabs-only-field asserted nothing. musterr does not extract a
leading KEY=value the way trace does, so bash failed to find a command named
DATABRICKS_BUNDLE_ENGINE=terraform, musterr accepted that as the expected
failure, and the contains.py check never matched. Both tests also kept an
out.requests.txt golden carrying per-run timings.

Co-authored-by: Isaac

The defect was described in a script comment, which does not reach the golden.
Using the repo's convention puts it in output.txt where a reader of the test
output sees it, alongside a short note on the cause.

Co-authored-by: Isaac

…inel

reference-dabs-only-field only showed terraform rejecting the reference, so the
conversion never ran. Both jobs now deploy with a literal and the reference is
introduced afterwards, pointing a DABs-only field at another resource's
DABs-only field. Neither is in terraform.tfstate, so both resolution methods
fail and the conversion reports which field and which template:

  jobs.dst field tasks[0].new_cluster.autotermination_minutes:
  method A: jobs: "..." is a DABs-only field with no Terraform equivalent;
  method B: cannot look up "..." : jobs: "..." is a DABs-only field ...

It goes through the migrate command, since a deploy carrying the reference is
rejected by terraform before the conversion runs.

tfstate-version-unsupported no longer gates its post-deploy script on a sentinel
file. Commenting the script out in databricks.yml and enabling it with
update_file.py is the idiom the other migrate tests already use, and leaves the
script itself unconditional.

Both tests were also keeping an out.requests.txt golden carrying per-run timings.

Co-authored-by: Isaac

Using bundle.engine: direct rather than the migrate command puts this through
the automatic migration, so it reports telemetry for both populations — a failed
dry run before opting in, and direct_migrate_error after — each with the saferr:

  jobs.*: cannot resolve field %q (template %q): jobs.%s field %s:
  method A: %w; method B: cannot look up %q: %w

It also corrects the reason the previous version went through the command: a
deploy carrying this reference is not rejected by terraform. The field has no
Terraform equivalent, so it is dropped on conversion along with the reference
inside it, which is what leaves the conversion as the only thing that fails. No
literal-then-swap is needed either; the reference can be there from the start.

This is the reference-resolution failure class reaching telemetry end to end,
which the unit table previously covered alone.

Co-authored-by: Isaac

… error

Reformatting only, except for one real gap: substitute's branch for a %w whose
argument holds no error was untested. A nil error interface matches no case in
templateArgs, so nothing is retained and the verb stays in the template.

Errorf stays below 100% because of the vet printf marker, which is unreachable
by construction.

Co-authored-by: Isaac

safeerr's format scanner rejected an explicit argument index only ahead of a
width or precision, so %2[2]s and %.2[2]s parsed '[' as the verb and the
template desynchronized from fmt. The whole design rests on bailing out of
anything the scanner does not model, and this was a hole in that.

Following a template through %w was unbounded, so an error whose Unwrap reaches
back to an ancestor recursed until the stack was gone. Bounded instead.

An error retained for %w chaining now also contributes its stand-in under an
ordinary verb, so a typed error reports its classification whether a call site
writes %s or %w.

ResourceKey.SafeString read the group straight out of the key without checking
it, so a key of an unexpected shape would have put its second segment into a
telemetry field. It now reports only a group this package defines.

The secret scope fixups iterated a map while returning the offending key, which
travels to a diagnostic and to telemetry, so two invalid scopes made the output
depend on map order. Pre-existing, and the extraction did not change it, but
sorting is what the repo's determinism rule asks for.

Co-authored-by: Isaac

Nothing covered them. The strings BuildStateFromTF produces are unit-tested, but
the fields carrying them to telemetry were not, so a renamed or duplicated json
tag would silently stop populating a dashboard column rather than fail anything.

This does not cover direct_migrate_warning_saferr end to end. Producing a
conversion warning needs the two resolution methods to disagree, which needs a
crafted terraform.tfstate: a real deploy stores the same value on both sides, and
name_prefix does not help because it is applied before references resolve.

Co-authored-by: Isaac

direct_migrate_warning_saferr had no coverage past the string BuildStateFromTF
returns. A post-deploy script edits dst's stored name in terraform.tfstate, so
Method A reads "source-drifted" from dst's own state while Method B evaluates
${resources.jobs.src.name} to "source". They disagree, the conversion warns, and
the migration stops without an error:

  direct_migrate_warnings true
  direct_migrate_warning_saferr jobs.%s field %q: method A and method B disagree

Editing the stored value is the point. A real deploy stores the same string on
both sides of a name-to-name reference, and name_prefix does not help because it
is applied before references resolve, so config alone cannot produce this.

Co-authored-by: Isaac